Pull Request Review: Add POST /ingredicheck/v2/scan/{scanId}/image endpoint

I've thoroughly reviewed this PR against the codebase patterns, security requirements, and database schema. Here's my detailed feedback:

✅ Strengths

1. Excellent Security Implementation

• ✅ Proper ownership verification by checking scan existence before upload
• ✅ Works correctly with RLS policies (scan_images and storage.objects policies enforce user_id = auth.uid())
• ✅ 10MB file size limit enforced at form-data parsing level
• ✅ Content hash (SHA-256) prevents duplicate uploads with UNIQUE constraint
• ✅ Storage path structure ({year}/{month}/{scanId}/{contentHash}.jpg) prevents path traversal attacks

2. Robust Error Handling

• ✅ Consistent error response format: { error: 'message' }
• ✅ Proper HTTP status codes (400, 404, 500, 201, 200)
• ✅ Try-catch wraps form-data parsing
• ✅ Excellent cleanup: Failed database inserts clean up uploaded storage files (scan.ts:428-432)
• ✅ Console logging follows codebase pattern: [scan#uploadImage]

3. Smart Duplicate Detection

• ✅ Returns 200 with existing record if image already uploaded (same content hash)
• ✅ Updates last_activity_at even for duplicates
• ✅ Efficient: checks DB before uploading to storage

4. Proper Activity Tracking

• ✅ Updates scans.last_activity_at on image upload
• ✅ Increments scans.images_processed counter
• ✅ Creates scan_images record with status='pending' for async processing

5. Follows Established Patterns

• ✅ Parameter validation matches other endpoints (scan.ts:330-335)
• ✅ Database error handling consistent with reanalyze() (scan.ts:310-316)
• ✅ Form-data parsing matches lists.ts pattern
• ✅ Response structure consistent with existing endpoints

⚠️ Issues & Recommendations

CRITICAL: Missing UUID Validation

Issue: The scanId parameter is not validated as a valid UUID format before database query.

Risk: Malformed UUIDs could cause database errors that leak stack traces.

Evidence: Other endpoints validate UUIDs:

// family.ts:55-58
function isValidUuid(value: unknown): boolean {
    if (typeof value !== 'string') return false
    return /^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$/.test(value)
}

Recommendation: Add validation at scan.ts:331:

if (!scanId || !isValidUuid(scanId)) {
    ctx.response.status = 400
    ctx.response.body = { error: 'Invalid scanId format' }
    return
}

MEDIUM: Missing Content-Type Validation

Issue: The code accepts any contentType from the uploaded file without validation (scan.ts:404).

Risk:

• Non-image files could be uploaded (e.g., HTML, SVG with scripts)
• Incorrect file extensions (.jpg hardcoded but could be PNG, WebP, etc.)

Current Code:

const imageFile = formData.files?.find((file: any) => 
    file.name === 'image' || file.contentType?.startsWith('image/')
)

Recommendation: Add strict content-type validation at scan.ts:350 (after form parsing):

const ALLOWED_TYPES = ['image/jpeg', 'image/jpg', 'image/png', 'image/webp']
const contentType = imageFile.contentType || 'image/jpeg'

if (!ALLOWED_TYPES.includes(contentType)) {
    ctx.response.status = 400
    ctx.response.body = { error: 'Invalid image type. Allowed: JPEG, PNG, WebP' }
    return
}

// Fix file extension based on actual content type
const ext = contentType === 'image/png' ? 'png' : 
            contentType === 'image/webp' ? 'webp' : 'jpg'
const storagePath = `${year}/${month}/${scanId}/${contentHash}.${ext}`

MEDIUM: Missing Image Dimension/Size Validation

Issue: No validation of actual image dimensions or file size beyond 10MB limit.

Risk:

• 1px x 1px images accepted (not useful for scanning)
• 10MB limit might be too large for mobile uploads (network cost, processing time)

Recommendation: Add validation at scan.ts:360 (after getting imageData):

// Validate actual file size (not just form limit)
const sizeInMB = imageData.byteLength / (1024 * 1024)
if (sizeInMB > 5) {  // Stricter limit for actual image
    ctx.response.status = 400
    ctx.response.body = { error: 'Image too large. Maximum 5MB.' }
    return
}

// Optional: Validate dimensions using image decoder
// (Would require adding image processing library)

LOW: Inconsistent Error Handling for Storage Upload

Issue: Storage upload errors are logged but return generic "Failed to upload image" (scan.ts:410-414).

Current Code:

if (uploadResult.error) {
    console.error('[scan#uploadImage] storage upload error', uploadResult.error)
    ctx.response.status = 500
    ctx.response.body = { error: 'Failed to upload image' }
    return
}

Recommendation: Add specific error handling for common cases:

if (uploadResult.error) {
    console.error('[scan#uploadImage] storage upload error', uploadResult.error)
    
    // Check for specific storage errors
    if (uploadResult.error.message?.includes('Duplicate')) {
        // This shouldn't happen due to earlier check, but handle gracefully
        ctx.response.status = 409
        ctx.response.body = { error: 'Image already exists' }
        return
    }
    
    ctx.response.status = 500
    ctx.response.body = { error: 'Failed to upload image' }
    return
}

LOW: Magic Number for File Size

Issue: 10 * MB is used inline (scan.ts:339) instead of a named constant.

Current Code:

const MB = 1024 * 1024  // Line 321
formData = await body.value.read({ maxSize: 10 * MB })  // Line 339

Recommendation: Define at top of file:

const MB = 1024 * 1024
const MAX_IMAGE_SIZE_MB = 10  // or 5 based on recommendation above

Then use:

formData = await body.value.read({ maxSize: MAX_IMAGE_SIZE_MB * MB })

LOW: Potential Race Condition in Duplicate Check

Issue: Between checking for duplicates (scan.ts:365-373) and inserting (scan.ts:417-425), another request could insert the same image.

Risk: Low (database UNIQUE constraint will catch it), but error handling could be clearer.

Recommendation: Wrap insert in try-catch and detect UNIQUE violation:

try {
    const imageInsertResult = await ctx.state.supabaseClient
        .from('scan_images')
        .insert({
            scan_id: scanId,
            content_hash: contentHash,
            status: 'pending',
            storage_path: storagePath
        })
        .select()
        .single()
    
    if (imageInsertResult.error) {
        // Check if it's a UNIQUE constraint violation
        if (imageInsertResult.error.code === '23505') {  // PostgreSQL unique_violation
            // Another request uploaded same image concurrently
            await ctx.state.supabaseClient.storage
                .from('scan-images')
                .remove([storagePath])
            
            // Fetch and return the existing record
            const existing = await ctx.state.supabaseClient
                .from('scan_images')
                .select('*')
                .eq('scan_id', scanId)
                .eq('content_hash', contentHash)
                .single()
            
            ctx.response.status = 200
            ctx.response.body = {
                id: existing.data?.id,
                contentHash: existing.data?.content_hash,
                status: existing.data?.status,
                storagePath: existing.data?.storage_path,
                queuedAt: existing.data?.queued_at
            }
            return
        }
        
        throw imageInsertResult.error
    }
} catch (error) {
    console.error('[scan#uploadImage] insert error', error)
    // Clean up uploaded file
    await ctx.state.supabaseClient.storage
        .from('scan-images')
        .remove([storagePath])
    ctx.response.status = 500
    ctx.response.body = { error: 'Failed to create image record' }
    return
}

SUGGESTION: Add Tests

Missing: No test coverage for this endpoint.

Recommendation: Add tests covering:

1. ✅ Successful image upload
2. ✅ Duplicate image detection
3. ✅ Invalid scanId (missing, malformed UUID)
4. ✅ Scan not found (404)
5. ✅ Missing image file (400)
6. ✅ File too large (400)
7. ✅ Invalid content type (400)
8. ✅ Storage upload failure (500)
9. ✅ Database insert failure with cleanup verification
10. ✅ Concurrent duplicate uploads (race condition)

SUGGESTION: Add API Documentation

Missing: No OpenAPI/Swagger documentation for this endpoint.

Recommendation: Document:

• Request: multipart/form-data with image field
• Accepted formats: JPEG, PNG, WebP
• Max size: 5-10 MB
• Response schemas for 200, 201, 400, 404, 500
• Example curl command

📊 Performance Considerations

✅ Good

• Efficient duplicate check before storage upload
• Content hash computation uses native crypto.subtle (fast)
• Database indexes exist on scan_images(scan_id, queued_at) for queries

⚠️ Consider

• Large images: 10MB uploads could impact:

  • API response time (upload + hash + storage)
  • Storage costs
  • Downstream AI processing time

  Recommendation: Consider image compression/resizing before storage or enforce stricter limits.

🔒 Security Assessment

Recommendation: Add rate limiting to prevent abuse:

// Example: limit 20 uploads per scan per minute
// Could use Redis or database-based rate limiting

📝 Code Quality

✅ Excellent

• Clear function names (uploadImage, generateContentHash)
• Consistent code style with codebase
• Good separation of concerns
• Proper TypeScript types

⚠️ Minor Issues

• Magic number 10 for MB limit
• any type for imageFile (scan.ts:348): formData.files?.find((file: any) => ...)
  • Recommendation: Define proper type or use unknown

🎯 Test Coverage Needed

Based on feedback.ts testing patterns, add:

// test/scan-upload.test.ts
Deno.test('POST /scan/{id}/image - success', async () => { ... })
Deno.test('POST /scan/{id}/image - duplicate', async () => { ... })
Deno.test('POST /scan/{id}/image - invalid UUID', async () => { ... })
Deno.test('POST /scan/{id}/image - unauthorized', async () => { ... })

📋 Checklist for Approval

Must Fix (CRITICAL)

• Add UUID validation for scanId

Should Fix (MEDIUM)

• Add content-type validation (JPEG, PNG, WebP only)
• Fix hardcoded .jpg extension based on actual content type
• Consider stricter file size limit (5MB instead of 10MB)

Nice to Have (LOW)

• Add test coverage
• Add API documentation
• Define MAX_IMAGE_SIZE_MB constant
• Add rate limiting
• Handle UNIQUE constraint race condition explicitly
• Add image dimension validation

🏁 Overall Assessment

Rating: 7.5/10 - Good implementation with room for improvement

Strengths:

• Excellent security foundation (RLS, ownership checks, cleanup)
• Follows codebase patterns consistently
• Smart duplicate detection
• Proper error handling structure

Weaknesses:

• Missing input validation (UUID, content-type)
• No test coverage
• No rate limiting

Recommendation: APPROVE with changes - Fix the critical UUID validation issue and medium-priority content-type validation before merging. Other suggestions can be addressed in follow-up PRs.

Great work on implementing this endpoint! The core logic is solid, and with a few validation improvements, this will be production-ready. Let me know if you need any clarification on these recommendations.